-
Notifications
You must be signed in to change notification settings - Fork 14k
bootstrap: respect build.python on macOS
#148636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
The `python()` method was hardcoded to return `/usr/bin/python3` on macOS, ignoring the `build.python` config option. This change respects the config while maintaining the system Python as the default.
|
r? @clubby789 rustbot has assigned @clubby789. Use |
|
I've tested the default is set locally (x86_64-apple-darwin), but it'd be great if a CI job on an *-apple-darwin worker could also confirm. |
|
@bors try jobs=aarch64-apple |
This comment has been minimized.
This comment has been minimized.
bootstrap: respect `build.python` on macOS try-job: aarch64-apple
src/bootstrap/src/lib.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both branches are now much more similar than before your change, which makes me wonder where the difference went. Or maybe the default is good for most non-Windows systems generally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR actually makes them more consistent, in that now for *-apple-darwin vs non *-apple-darwin targets we both first try to use an explicitly configured build.python, then we try our luck of discovering an environmental Python. Or in the case of *-apple-darwin, the matter of "environmental python" is currently still hijacked due to lldb reasons cf. #148361 #123621 #134682.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which I agree is good, but makes me think perhaps we can go even further then? "/usr/bin/python3/ is a good default on many (most?) linux systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't have build.python set and bootstrap still works for me on Linux, so some other place also sets a default python?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would work for all cases, and I would prefer to keep the scope of this PR focused. I think we'd be open for a follow-up PR, though I cannot say I know for sure /usr/bin/python3 is universally valid. For instance, picking up a python3 from PATH is also a reasonable choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I don't have build.python set and bootstrap still works for me on Linux, so some other place also sets a default python?
It's ultimately set here:
https://github.com/rust-lang/rust/blob/1.91.0/src/bootstrap/src/core/sanity.rs#L166
Note that BOOTSTRAP_PYTHON is the interpreter invoking bootstrap.py, which is itself called by x.py. x.py can actually choose a different Python than what it's invoked with, e.g. switching from Python 2 if it finds a Python 3 install:
https://github.com/rust-lang/rust/blob/1.91.0/x.py#L18
build.python should be available to override all of that guesswork, except it (and all of the indirection) were ignored by a hardcoded value on macOS. This PR fixes that particular issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @xSetech! So that makes me think finding the right python is already happening in sanity.rs, except it would fail is the PATH does not have "/usr/bin/" at higher priority than some other path that contains a python install. What does your path look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just removed both branches and inlined the .expect call at call sites, which seems much cleaner given what sanity.rs as a module already does for other tools.
@hkBst The logic in sanity.rs isn't guaranteed to choose a Python version that actually works. Python and LLVM from Homebrew are first in my path and those are the correct choices for my particular setup, so I will ultimately set build.python and override the choice sanity.rs would otherwise make. Given all that, I think there could be a bigger discussion about the heuristics it applies, and perhaps a follow-up PR might better extend the set of checks performed in sanity.rs (e.g. to verify that any chosen Python can actually load the LLDB module).
|
This PR modifies If appropriate, please update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @xSetech! I'll r+ once the try job comes back.
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 37f35bb failed: CI. Failed jobs:
|
|
Curious, I'll take a look tmrw at how they are wired together 🤔 |
|
|
The choice of default for macOS was moved to sanity::check()
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Tidy CI job failures were widespread, see: |
|
I wonder how viable it is to change debuginfo tests to use lldb's embedded Python (via the lldb |
To answer my own question, it seems very viable: If we can consistently use LLDB's embedded Python, that should make |
The
python()method was hardcoded to return/usr/bin/python3on macOS, ignoring thebuild.pythonconfig option. This change respects the config while maintaining the system Python as the default.